Fix GH-22060 and GH-22122: pin object/closure in callback dispatch#22151
Fix GH-22060 and GH-22122: pin object/closure in callback dispatch#22151iliaal wants to merge 1 commit into
Conversation
Pin object and closure across zend_call_known_fcc and spl_perform_autoload so a callback that releases the borrowed FCC (autoloader self-unregister, SQLite3 setAuthorizer(null)) doesn't free $this mid-call. Initialize fcc.closure in ReflectionFunction::invoke/invokeArgs since the pin reads it. Fixes phpGH-22060 Fixes phpGH-22122
DanielEScherzer
left a comment
DanielEScherzer
left a comment
There was a problem hiding this comment.
okay for ext/reflection, don't know about the other parts
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
I was trying to understand why we would need to add ref the fcc.object, and my understanding is that it represents the $this value, so I guess this is valid.
However, we are once again fixing bugs and affecting performance for idiosyncratic code. I would rather prefer a solution that bans unsetting such callables within the calls. Similar to a fix we recently did for an unserialising routine in SPL.
|
Pinning is two refcount ops against a full call-frame setup, so the per-call cost is negligible. Banning is the bigger change: the autoload loop is deliberately built to allow add/remove mid-iteration, and disabling the authorizer mid-call is a one-shot the sqlite3 test relies on. Turning either into a thrown error is a behavior change that can't go in a stable branch. If the stricter semantics are worth having, that's a master-only follow-up. |
Such things will come up more and more with the raise of AI. A while ago when fixing a similar issue, @TimWolla asked me if This should target master anyway (we agreed to not fixing such bugs on stable branches that are unlikely to cause issues in the real world). So maybe let's just go with the above instead. |
4 participants
GH-22060 + GH-22122 fix for PHP-8.4. Same UAF in two callback-dispatch sites: zend_call_known_fcc and spl_perform_autoload forward the borrowed object/closure into the call frame without addref. 8.4 and 8.5 both need the pair, since SPL autoload still uses zend_call_known_function direct. Master only needs the zend_API change because Zend/zend_autoload.c routes through zend_call_known_fcc.